fix(review): honour glossary-path, and make a custom glossary an override (#146)#151
Conversation
…ride (#146) `runReview` read the built-in glossary file directly and never looked at `inputs.glossaryPath`, so a repo with a custom glossary had its translation produced against one terminology and judged against another. The failure was silent — the run logged `✓ Loaded glossary for <lang>` either way — and it mattered more after v0.22.0 made `terminology` a gating category: a reviewer judging against the wrong glossary suppresses the auto-merge gate on correct work and biases the Stage 4 shadow calibration the per-criterion floors will be chosen from. Review now goes through the same `loadGlossary` that sync and rebase use. Fixing that surfaced a deeper defect in the shared loader, which the issue did not describe: the custom path was tried AFTER the built-in one, so for every language that ships a glossary — zh-cn, fa, fr, the whole estate — `glossary-path` was silently ignored in all three modes, not just review. An unreadable custom file also quietly degraded to built-in terminology. - a custom glossary is now an override: the only candidate when set - a path that cannot be read or parsed fails the run rather than falling back to terminology the operator did not ask for - the mode logs which glossary it used, custom or built-in No production edition sets `glossary-path`, so nothing in the fleet changes behaviour today. Also fixed while in there: a repository whose name carries no language suffix reviewed with no glossary and said nothing — it now warns. The reviewer's glossary-to-prompt formatting moved out of src/index.ts, which `import.meta.url` puts beyond the Jest CJS registry and so beyond testing, into `formatGlossaryTerms` with tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes review-mode glossary handling so review verdicts are computed against the same terminology configuration used during translation. It routes review glossary loading through the shared loadGlossary helper, changes glossary-path to behave as a strict override (and fail fast when unreadable), and extracts reviewer glossary prompt formatting into a testable helper.
Changes:
- Make
glossary-pathan override in the shared glossary loader and fail the run when a configured custom glossary can’t be read/parsed. - Update review mode to use the shared glossary loader and shared prompt formatting, and warn when target language can’t be inferred from the repo name.
- Add unit tests for the updated loader contract and for glossary prompt formatting; update docs and changelog accordingly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/sync-orchestrator.ts |
Updates loadGlossary semantics (custom override + fail-fast) and adds formatGlossaryTerms helper for reviewer prompt formatting. |
src/index.ts |
Updates runReview to use loadGlossary(inputs.glossaryPath) and formatGlossaryTerms; adds warning when language suffix can’t be detected. |
src/__tests__/sync-orchestrator.test.ts |
Adds tests pinning the new glossary precedence/fail-fast behavior and tests for formatGlossaryTerms. |
docs/user/glossary.md |
Documents glossary-path as an override and highlights the need for consistent configuration across modes. |
docs/user/action-reference.md |
Updates glossary-path description to reflect override + fail-fast behavior across sync/rebase/review. |
dist-action/index.js |
Updates the built action bundle to include the new glossary loading/formatting behavior. |
CHANGELOG.md |
Adds an unreleased entry describing the glossary-path/review-mode fixes and related behavior change. |
Comments suppressed due to low confidence (1)
src/sync-orchestrator.ts:153
- The built-in glossary branch doesn’t validate that
glossary.termsis an array before loggingglossary.terms.lengthand returning it. If the built-in JSON is malformed (or the schema changes), this can either throw later (e.g., informatGlossaryTerms) or return an invalidGlossaryobject. Validate the schema here the same way the custom glossary path is validated, so failures are caught early with a clear message.
try {
const content = await fs.readFile(builtInPath, 'utf-8');
const glossary: Glossary = JSON.parse(content);
if (glossary) {
logger?.info(
`✓ Loaded built-in glossary for ${targetLanguage} with ${glossary.terms.length} terms`
);
return glossary;
}
… too Addresses Copilot's review of #151. 1. The thrown message for an unreadable custom glossary interpolated the caught Error object, so run()'s `Action failed: ${error.message}` wrapper produced a doubled prefix: Action failed: Could not load custom glossary from X: Error: ENOENT… (Copilot predicted "[object Object]"; that needs a thrown plain object, which fs.readFile and JSON.parse never produce. The doubled prefix is the real symptom.) 2. The built-in branch checked only `if (glossary)` while the custom branch validated `Array.isArray(glossary.terms)` — an asymmetry introduced when the custom branch was hardened. A glossary whose `terms` was present but not an array logged "with undefined terms" and was RETURNED, then failed downstream in formatGlossaryTerms or the translator, far from the cause. Both branches now go through readGlossaryFile, keeping the validation identical while leaving the consequence deliberately different: a bad custom path is operator misconfiguration and fails the run, a corrupt built-in glossary is a broken install and warns. The error helper is duck-typed rather than using this codebase's `error instanceof Error` idiom, and that is load-bearing: fs.promises rejections cross a realm boundary under Jest's vm context, so `instanceof Error` is FALSE for them despite constructor.name being Error — the idiom falls through to String(error) and re-adds the very prefix it exists to strip. Cosmetic-only in production (single realm), but it is the difference between this being testable and not. The regression test caught exactly that while the first version of this fix was in place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed Copilot's review in 7e39bfb — the one inline comment, plus the suppressed low-confidence note, which was the better of the two findings. Inline (error interpolation) — valid, fixed; replied on the thread. The stated symptom was inaccurate ( Suppressed note (built-in branch skipped the Worth recording from the fix: the error helper duck-types on 54 suites, 1279 passing. |
* chore: release v0.23.0 Bundles the three "fix before shadow mode" blockers, all harness-validated: - #150 (#149) — forward/init resolved the glossary CWD-relative and no edition repo carries one, so a resync from the target repo translated with no glossary and logged nothing either way. Now package-relative, reported, loud on failure; `forward --glossary` added. - #151 (#146) — review mode never read `glossary-path`, so a repo with a custom glossary was translated against one terminology and judged against another. Fixing it exposed that the shared loader tried the built-in glossary FIRST, so `glossary-path` was silently dead in all three modes for every language that ships one. Custom is now an override that fails the run when unreadable. - #152 (#148) — the four diffChecks gate absolutely but were all model output. structurePreserved and headingMapCorrect are now computed; scopeCorrect and positionCorrect stay model-asserted, are tagged in a new optional `diffCheckSources` field, and gate through a `diff-check` finding. Routing is unchanged; shadow calibration can now separate measured fact from model opinion. No behavioural change on upgrade for the fleet: no edition sets `glossary-path`, `auto-merge-mode` still defaults to `off`, and the deterministic checks were verified against every production file before shipping (248 of 249 with sections carry a heading map; the one gap was real and is fixed in lecture-python-programming.fa#142). schemaVersion stays 1 — `diffCheckSources` is a pure optional addition. Pre-flight: rebuilt dist-action matches the committed bundle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: re-emit the pull_request event (empty; squashed away on merge) CI produced no run for 6439ad0 on either `opened` or `reopened`, and no skipped-run record either. Ruled out first: paths-ignore (PR #145 fired with an identical five-file set including .dev/STATE.md, on its own release commit), Actions disabled, workflow inactive, draft PR, a push/create race, skip-ci tokens, and workflow-file drift. This empty commit tests whether a fresh head SHA is evaluated at all. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes #146.
What was wrong
runReviewbuilt its glossary by reading the built-in file directly and never looked atinputs.glossaryPath— whichgetReviewInputs()had been parsing all along and handing over unused. So on a repo with a custom glossary, the translation was produced against one terminology and judged against another, silently: the run logged✓ Loaded glossary for <lang> with N termseither way, so the only symptom was puzzling terminology findings.Since v0.22.0 that isn't just noise.
terminologyis a gating category — oneminorfinding blocksauto-merge— so a reviewer on the wrong glossary suppresses the gate on correct work, and biases the Stage 4 shadow calibration that the per-criterion floors will be chosen from.The deeper defect underneath it
Fixing this meant routing review through the shared
loadGlossaryinsync-orchestrator.ts— the loader #146 describes as already doing it properly. It isn't. It tried the built-in glossary first and treated the custom path as a fallback:So
glossary-pathwas silently dead in all three modes, not just review — for every language with a built-in glossary, which is the entire estate. And an unreadable custom file quietly degraded to built-in terminology rather than complaining. Both are the same silent-wrong-glossary class as #149.That is a behaviour change beyond the issue's scope, so stating it plainly: a custom glossary is now an override (the only candidate when set), and a path that cannot be read or parsed fails the run. I checked every edition's workflows — no repo in the fleet sets
glossary-path, and the issue says the same, so nothing changes behaviour in production today. The alternative — leaving custom-as-fallback and just pointing review at it — would have made review consistent with sync while leaving custom glossaries just as ignored, which fixes the symptom the issue names and not the thing that would bite the first edition to adopt one.Also in this change
detectTargetLanguage()returns undefined and the whole block was skipped in silence. It now warns that terminology findings are unreliable.src/index.tsintoformatGlossaryTerms.src/index.tsusesimport.meta.url, which the Jest CJS registry cannot load, so nothing in that file can be unit tested — the same trapsrc/rebase-siblings.tsdocuments, and the reason this formatting had no tests.Verification
Full suite green: 54 suites, 1277 passing (6 pre-existing skips); lint and format clean. Nine tests over the loader and formatter, including the two that pin the new contract — custom beats built-in, and an unreadable custom path throws instead of falling back.
Not yet exercised on the harness. This and #149 will be tested together there before v0.23.0 is cut, which is the point of holding the tag.
Sequencing
Both halves of the glossary defect are now fixed — #149 (resync path resolved the glossary CWD-relative) and #146 (review path ignored it). NEXT-STEPS records both as needing to land before shadow mode, since each biases the calibration data Stage 4 depends on. #148 (diffChecks are model output but gate absolutely) is the remaining one on that list and is untouched here.
🤖 Generated with Claude Code